Skip to content

Conversation

@ashakirin
Copy link
Contributor

Added tools name format validation accordingly #SEP-986
Note: tool duplications are already checked (in McpAsyncServer.java and in McpStatelessAsyncServer.java)

@Kehrlann Kehrlann self-requested a review January 30, 2026 08:58
@Kehrlann
Copy link
Contributor

Thanks for your contribution @ashakirin ! A few comments.

Optional validation

The spec specifically says that tools SHOULD follow the naming conventions (Server > Tools > Tool names). They MAY not follow these, and so we should allow invalid names (although not by default).
We could consider a per-server validator, that you plug in the SyncSpecification and AsyncSpecification. Thoughts?

Client-side validation

I like the approach on the client side, validate but don't fail. The spec itself is super vague, "client can validate" which has no RFC meaning. Maybe we shouldn't even validate on the client side.

If we chose to validate, in the current implementation the log is very verbose if you're writing a client and the server (that you don't control) has an invalid tool name. Every time you make a "call tool request" you get a WARN-level log. I think that's too much, and the level is probably too high. In that case, keep the logs for the "list tool" request but not "call tool".

@ashakirin
Copy link
Contributor Author

@Kehrlann: Thanks for detail review, Daniel!
My thoughts:

Optional validation

The spec specifically says that tools SHOULD follow the naming conventions (Server > Tools > Tool names). They MAY not follow these, and so we should allow invalid names (although not by default). We could consider a per-server validator, that you plug in the SyncSpecification and AsyncSpecification. Thoughts?

My initial interpretation of SHOULD was that the SDK may choose whether to support validation at all, not that validation of server-side tool names should be optional. That seems to be incorrect.

Proposal: using different validation options for different MCP servers registered in the same application is probably not a very typical use case. I would therefore propose a global setting to disable validation, with an option to override it per server in SyncSpecification, AsyncSpecification, StatelessAsyncSpecification, and StatelessSyncSpecification. WDYT?
This keeps the default behavior simple while still allowing flexibility when integrating with non-compliant servers. What do you think?

Client-side validation

I like the approach on the client side, validate but don't fail. The spec itself is super vague, "client can validate" which has no RFC meaning. Maybe we shouldn't even validate on the client side.

If we chose to validate, in the current implementation the log is very verbose if you're writing a client and the server (that you don't control) has an invalid tool name. Every time you make a "call tool request" you get a WARN-level log. I think that's too much, and the level is probably too high. In that case, keep the logs for the "list tool" request but not "call tool".

My idea was to validate tool names via deserialization on both the client and server sides: on the client during listTools, and on the server during tool/call.
I agree that the current implementation in compact constructors is too verbose, because:

  • the client constructs the CallToolRequest itself
  • the compact listTools constructor is invoked twice due calling of pagination / cursors constructor

Therefore I am going to move validation into McpAsyncClient.listToolsInternal().
Do you think it makes sense to validate tool names on the server side during tool/call? Basically a client can call server with non-existent and invalid tool name, but that will result in a “tool not found” error. And existing invalid server-side tool names will be already validated during tool registration on the server side - so additional validation seems doesn't add value.

@Kehrlann
Copy link
Contributor

Kehrlann commented Feb 3, 2026

Proposal: using different validation options for different MCP servers registered in the same application is probably not a very typical use case. I would therefore propose a global setting to disable validation, with an option to override it per server in SyncSpecification, AsyncSpecification, StatelessAsyncSpecification, and StatelessSyncSpecification. WDYT? This keeps the default behavior simple while still allowing flexibility when integrating with non-compliant servers. What do you think?

I like the idea, something like .skipToolNameValidation(boolean skip)?

Therefore I am going to move validation into McpAsyncClient.listToolsInternal().

Works for me.

Do you think it makes sense to validate tool names on the server side during tool/call? Basically a client can call server with non-existent and invalid tool name, but that will result in a “tool not found” error. And existing invalid server-side tool names will be already validated during tool registration on the server side - so additional validation seems doesn't add value.

No, don't validate client calls on the server. If a tool name does not exist, it does not exist - no matter the reason.
If a tool is present but has an invalid name, that means the user configured their server with "skip validation" and therefore we assume they know what they are doing and do not want extra messages.

@ashakirin
Copy link
Contributor Author

@Kehrlann: have updated PR accordingly our discussion, could you please take a look?

Copy link
Contributor

@Kehrlann Kehrlann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. I have one more ask.

Comment on lines 1778 to 1788
@Test
void testListToolsResultDeserializationWithInvalidToolName() throws Exception {
// Deserialization should not throw, just warn
String json = """
{"tools":[{"name":"invalid tool name","description":"test"}],"nextCursor":null}""";

McpSchema.ListToolsResult result = JSON_MAPPER.readValue(json, McpSchema.ListToolsResult.class);

assertThat(result.tools()).hasSize(1);
assertThat(result.tools().get(0).name()).isEqualTo("invalid tool name");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test showing it throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is obsolete and stays from the times when validation was in McpSchema. Removed.

ashakirin and others added 3 commits February 10, 2026 00:29
Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
Co-authored-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
…on in name, used nested tests and minor refactorings done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants